Add support for alternatiwe image formats like webp and avif - #1651
Add support for alternatiwe image formats like webp and avif#1651tito10047 wants to merge 23 commits into
Conversation
…update handling of modern image formats (WebP, AVIF).
…tiation with tests.
…ative` and add comprehensive tests for alternative format handling.
…otiator` initialization, and enhance support for format-specific mime types and post-processors.
…pdate related methods, and improve support for flexible format negotiation.
…native_formats`, deprecate `isWebpSupported`, and update service definitions.
|
Hi @dbu . can you help me with cs fixer? I run cs fixer as you mentioned me last time, but nothing is fixed in me setup Thx |
webp configuration in favor of alternative_formats and ……agic driver can convert to avif, imagick convert to avif before postProcessor. so I add this option use_default_driver. only if this option is set to true (defaut) imagick will convert to avif. otherwise will be do it avifenc binary
|
Hi @dbu . Will you look at this PR please? |
|
Ok, But this PR is the same aproach as is already in this bundle. so nothing is changed. only some refactoring. |
dbu
left a comment
There was a problem hiding this comment.
i took a closer look now. the refactoring is well done and a much better and future proof way to achieve what the webp flag does. i am still unhappy that it deepens a pattern that i think we should move away from. in that way it sends a wrong signal to users who upgrade.
at the same time I don't have much time to work on the bundle, so it might be quite a while until i can address that. therefore lets merge this.
i commented on some details. can you please also add a changelog entry to explain the deprecation and the more generic solution, but also point out that client side resolving is the preferred way for supporting webp, avif etc?
| $webpGenerate = false | ||
| $alternativeFormats = false | ||
| ) { | ||
| if (\is_bool($alternativeFormats)) { |
There was a problem hiding this comment.
could we default the parameter to [] instead? otherwise not setting alternative formats triggers a deprecation
| $this->dispatcher = $dispatcher; | ||
| $this->defaultResolver = $defaultResolver ?: 'default'; | ||
| $this->webpGenerate = $webpGenerate; | ||
| $this->alternativeFormats = $alternativeFormats; |
There was a problem hiding this comment.
i would normalize this to have the property only accept array and translate false to [] and true to ['webp]. that way the BC is constrained to the constructor and the rest of the class simpler.
| * @param string $path | ||
| * @param string $filter | ||
| * @param string|null $resolver | ||
| * @param bool $webpSupported |
There was a problem hiding this comment.
| * @param bool $webpSupported | |
| * @param bool $webpSupported deprecated in favor of $alternativeFormatsSupported |
| * @param string $path | ||
| * @param string $filter | ||
| * @param string|null $resolver | ||
| * @param bool $webpSupported |
There was a problem hiding this comment.
| * @param bool $webpSupported | |
| * @param bool $webpSupported deprecated in favor of $alternativeFormatsSupported |
| - name: Run PHPUnit tests | ||
| env: | ||
| SYMFONY_DEPRECATIONS_HELPER: max[self]=0 | ||
| SYMFONY_DEPRECATIONS_HELPER: disabled=1 |
There was a problem hiding this comment.
please don't change this setting.
| service('liip_imagine.filter.manager'), | ||
| service('liip_imagine.cache.manager'), | ||
| '%liip_imagine.webp.generate%', | ||
| '%liip_imagine.webp.options%', |
There was a problem hiding this comment.
is there no risk of BC break? does the extension integrate the parameters into alternative_formats if the old parameters are set?
|
Hi, i can preprare new PR, with idea you want. So lets freeze this PR and I will do on new one. Ok? You want say me something what os your idea closer? But... When i will do new PR and abamdonenthis idea, there will must be BC. So what is better, accpet this with non BC or release new version with BC. I like second option. |
|
if you can address my comments here, we can merge this pull request, then at least there is a better support for the server-side negotiation. i think there is not much work left to do, and then we have an improvement we can release. |
|
Sorry for the delay, I had plent of new projects to do :) |
| @@ -1,84 +1,63 @@ | |||
| parameters: | |||
|
|
|||
There was a problem hiding this comment.
please revert these whitespace changes, the whitespace is for readability
| /var/ | ||
| /vendor/ | ||
| /.idea/ | ||
| /.junie/guidelines.md |
There was a problem hiding this comment.
please do not add local dev environment specific things to the gitignore. use your global gitignore on your workstation for those.
|
you closed some of my comments without response. i reopened those. please adress them so that we have a smooth upgrade path. |
…update handling of modern image formats (WebP, AVIF).
Description
This PR refactors the way modern image formats (like WebP and AVIF) are handled within the bundle. Instead of having hardcoded logic for a single format, it introduces a generic Alternative Formats system.
Key changes:
alternative_formatsconfiguration tree.webpconfiguration is automatically normalized into the new structure using abeforeNormalizationstep inConfiguration.php.FormatNegotiatorservice to dynamically select the best image format based on the browser'sAcceptheader (supporting AVIF, WebP, etc.).FilterPathContainerto support any alternative format, while markingcreateWebp()as deprecated.This approach follows the discussion in issue #1314, providing a future-proof solution for modern image delivery while ensuring zero breaking changes for existing installations.
Proff of concept here: just6words.com